Hide members of confidential projects #1031
Conversation
C# Unit Tests66 tests 66 ✅ 6s ⏱️ Results for commit c7015d1. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Have you tested this out manually? I suspect this will throw an auth error when users try to view a project they don't have permission to view the members of. The Access project users requirement will prevent queries of the user list, this is fine, but then the front end needs to know not to query for users which it can't know in the project load function.
|
just tested this out and it doesn't work, the user gets an error trying to access a project that's confidential if they're not a manager. |
|
@rmunn where's this PR at? |
Only the following people can see the list of members if a project is confidential: the manager(s) of the project, site admins, and the org admin(s) of the org(s) the project belongs to, if any. Everyone else, including people who are themselves members of the project, cannot see its membership.
rmunn
force-pushed
the
feat/hide-members-of-confidential-projects
branch
from
d555fa6
to
1aff610
Compare
GQL query will return an empty users list if you're not allowed to see the project members.
|
@hahn-kev - Picked this PR up again and made it work. What do you think? |
backend/LexBoxApi/GraphQL/CustomTypes/ProjectMembersVisibilityMiddleware.cs
Outdated
Show resolved
Hide resolved
backend/LexBoxApi/GraphQL/CustomTypes/ProjectMembersVisibilityMiddleware.cs
Outdated
Show resolved
Hide resolved
As suggested by C# builtin code analyzer.
For project members, and ONLY for project members, we will default to showing them for projects whose confidentiality has not been set. This isn't the most secure default, but it will avoid what would likely be a lot of user confusion.
|
@hahn-kev - Okay, I've gone ahead and implemented those changes, including the "members visible if confidentiality is not yet set" one. That one is a separate commit so it will be dead easy to revert later if we end up deciding this question the other way. Ready for re-review. |
UI unit Tests12 tests 12 ✅ 0s ⏱️ Results for commit c7015d1. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Personally, I'm happy with how you did this @rmunn 👍
Requesting changes, because as you can see in my comment I think there's some unused code to be removed. Correct me if I'm wrong.
Also, I found it a bit unsatisfying that we claim there are no users. So, I added a commit that you're free to challenge that makes 2 small changes:
- Preserve the current user in the list of members (otherwise it looks like they're not even a member and then they can also see their role)
- Add an indicator noting that the membership is confidential.
So, when the list has been filtered of other users it looks like this:
If I'm an org admin, project manager or site admin, it looks normal:
backend/LexBoxApi/Auth/Requirements/AccessProjectUsersRequirementHandler.cs
Outdated
Show resolved
Hide resolved
backend/LexBoxApi/GraphQL/CustomTypes/ProjectMembersVisibilityMiddleware.cs
Outdated
Show resolved
Hide resolved
Fixes #959.
Only the following people can see the list of members if a project is confidential:
Everyone else, including people who are themselves members of the project, cannot see its membership.
NOTE: After discussion, we have tentatively decided to make project members visible if the project's confidentiality is not set; i.e. you have to explicitly set a project to confidential before its members will be hidden. We can easily change that later if user feedback suggests it's needed.
Screenshots
What the admin or project manager sees if the project is confidential:
What non-manager members see: